feat: log when we see a new key we have never seen before #658
Conversation
…reverse engineering
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds debug logging to track unknown keys encountered during deserialization of Roborock data classes, logging each unique key only once to aid in future reverse engineering efforts.
- Added a class-level set
_missing_loggedto track which unknown keys have been logged - Implemented logging logic that emits a debug message when an unknown key is first encountered, including both original and decamelized key names
- Imported
ClassVartype hint to properly annotate the shared state
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class RoborockBase: | ||
| """Base class for all Roborock data classes.""" | ||
|
|
||
| _missing_logged: ClassVar[set[str]] = set() |
There was a problem hiding this comment.
The shared class variable _missing_logged is not thread-safe. If from_dict is called concurrently from multiple threads, race conditions could occur when checking membership and adding to the set. Consider using a threading.Lock or a thread-safe alternative like threading.RLock() to protect access to this shared state, or use a thread-safe data structure.
| if (log_key := f"{cls.__name__}.{key}") not in RoborockBase._missing_logged: | ||
| _LOGGER.debug( | ||
| "Key '%s' (decamelized: '%s') not found in %s fields, skipping", | ||
| orig_key, | ||
| key, | ||
| cls.__name__, | ||
| ) | ||
| RoborockBase._missing_logged.add(log_key) |
There was a problem hiding this comment.
The new logging behavior for unknown keys lacks test coverage. Consider adding a test that verifies: (1) a debug log is emitted when an unknown key is encountered for the first time, (2) the log is not emitted again for subsequent occurrences of the same key in the same class, and (3) the class name and both original and decamelized keys are included in the log message. The existing test_ignore_unknown_keys test could be extended to verify this behavior using a mock logger or caplog fixture.
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Could also consider storing this in diagnostic data for the traits to dump if thats also helpful.
A good follow up, I'll explore doing that as well |
This will help with future reverse engineering, we only log once